Skip to content

fix(sdk): request timeout, network-error normalization, idempotent-only retries, Retry-After, bounded iterate - #274

Merged
dodeja merged 4 commits into
mainfrom
fix/sdk-transport-resilience
Jul 1, 2026
Merged

fix(sdk): request timeout, network-error normalization, idempotent-only retries, Retry-After, bounded iterate#274
dodeja merged 4 commits into
mainfrom
fix/sdk-transport-resilience

Conversation

@dodeja

@dodeja dodeja commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Hardens the @terminal49/sdk transport layer against failure modes that can hang or corrupt a caller in production. Pure logic is extracted into small unit-testable modules behind the existing interceptor seam.

  • Request timeout — every request is bounded by an AbortController-based timeout (timeoutMs, default 30000, 0 disables). A hung upstream is aborted and rejected with a new TimeoutError. Applied once in Transport so it covers both the openapi-fetch client and the manual search() path. (new client/timeout.ts)
  • Network-error normalization + retry — a thrown fetch failure (DNS, ECONNRESET, "fetch failed", ...) is normalized to a Terminal49Error (NetworkError) and run through the same retry policy as a 5xx, via the RetryInterceptor.onError hook.
  • Idempotent-only retries — retries are gated to GET/HEAD (and writes that carry an Idempotency-Key). A POST/PATCH hitting a transient 5xx/network error is no longer silently replayed (which could create duplicate tracking requests). (new pure client/retry-policy.ts)
  • Retry-After — 429 backoff honors the server Retry-After header (delta-seconds or HTTP-date) instead of fixed exponential only.
  • Bounded iterate()BaseManager.createIterator stops at documented maxPages / maxRows caps so a no-op / overly-broad filter cannot walk the entire dataset. (managers/base.ts)
  • Search pathexecuteManual reads success bodies via readSuccessBody (new client/body.ts) so a non-JSON 200 is surfaced instead of being silently collapsed to undefined, and a thrown network error is normalized like the typed path.
  • Interceptor order — documented the load-bearing registration order (Retry registered last so it runs before error-mapping on the reverse onResponse/onError pass).

Behavior change worth a reviewer's eye

The previous SDK retried POST tracking-request creation on a 5xx; that is now intentionally blocked (duplicate-write risk). The corresponding test in client.test.ts was updated to document the new safe contract.

Issues

Closes DEV-10663

Green gate

All six gate commands pass (baseline SDK 51 pass/2 skip, MCP 77 pass):

command result
build @terminal49/sdk clean
build @terminal49/mcp clean
type-check @terminal49/sdk clean
type-check @terminal49/mcp clean
test @terminal49/sdk --run 80 pass / 2 skip (+29 new)
test @terminal49/mcp --run 77 pass

oxlint + oxfmt --check clean on changed files.

Notes

  • This is an AI-drafted PR for human review.
  • The err.message-leak fix lives in the MCP server (a separate PR); api/mcp.ts / packages/mcp are intentionally untouched here.

🤖 Generated with Claude Code

Greptile Summary

This PR hardens the @terminal49/sdk transport layer with five targeted reliability improvements: AbortController-based request timeouts, network-error normalization to NetworkError, idempotent-only retries (blocking unsafe replay of POST writes), Retry-After-aware backoff, and maxPages/maxRows caps on BaseManager.createIterator.

  • Timeout + normalization: withTimeout wraps every fetch (typed and manual paths) and a new toNetworkError helper ensures thrown fetch errors always surface as Terminal49Error subclasses rather than raw TypeErrors.
  • Idempotent-only retries: shouldRetryRequest gates retries to GET/HEAD/OPTIONS or writes carrying an Idempotency-Key, preventing duplicate tracking-request creation on transient 5xx — a behavioral change explicitly tested.
  • Iterator bounds: createIterator now stops at DEFAULT_ITERATE_MAX_PAGES (1000) and DEFAULT_ITERATE_MAX_ROWS (100k) so a broad filter cannot silently page the entire dataset.

Confidence Score: 3/5

Safe to merge once the uncaught-error gap in executeManual is addressed; the idempotency change is intentional and well-tested.

The manual fetch pipeline (executeManual, used by search()) normalizes network errors on the initial fetch but leaves an uncovered path: if the initial call returns a 5xx and the subsequent retry inside retry.onResponse itself throws a network error, that raw TypeError escapes to the caller without going through toNetworkError. Every other change in the PR is straightforward and well-tested. The Retry-After parsing also lacks an upper-bound cap, which could stall a process for an arbitrarily long period if the API returns a large delta-seconds value.

sdks/typescript-sdk/src/client/transport.ts (executeManual error handling path) and sdks/typescript-sdk/src/client/retry-policy.ts (Retry-After cap, TypeError breadth).

Important Files Changed

Filename Overview
sdks/typescript-sdk/src/client/transport.ts Adds timeout-wrapped fetch, manual pipeline (executeManual), and interceptor ordering. Network errors inside retry.onResponse retries escape the normalization guard.
sdks/typescript-sdk/src/client/retry-policy.ts New pure retry-policy helpers. Retry-After parsing has no upper-bound cap; isRetryableNetworkError matches all TypeError instances regardless of message.
sdks/typescript-sdk/src/client/interceptors.ts Adds onError hook to RetryInterceptor for network-error recovery, integrates idempotency gating, and adds Retry-After backoff. Logic and cleanup (finally-delete) are correct.
sdks/typescript-sdk/src/client/timeout.ts Clean AbortController-based timeout wrapper. Caller signal forwarding and timer cleanup in finally are correct.
sdks/typescript-sdk/src/client/managers/base.ts Adds maxPages/maxRows safety caps to createIterator. Row-count and page-count checks are correct; yields exactly maxRows items.
sdks/typescript-sdk/src/client/body.ts New readSuccessBody helper surfaces non-JSON and empty bodies correctly using response.clone().text() to avoid consuming the stream.
sdks/typescript-sdk/src/client/errors.ts Adds NetworkError and TimeoutError classes with proper instanceof Terminal49Error hierarchy for toNetworkError pass-through.
sdks/typescript-sdk/src/client.transport.test.ts New integration-level tests cover network-error retry, timeout abort, Retry-After delay, non-retry of POST, and iterate page cap.
sdks/typescript-sdk/src/client.test.ts Updated to document that POST writes are not retried on 5xx; test correctly verifies a single attempt and surfaces UpstreamError.

Fix All in Codex

Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
sdks/typescript-sdk/src/client/transport.ts:122-133
**Uncaught network error inside `retry.onResponse`**

If the initial fetch returns a 5xx and `retry.onResponse` enters its retry loop, any network error thrown by `this.fetchImpl(replayableRequest.clone())` inside that loop will propagate directly out of `retry.onResponse` without being caught. `executeManual` has no try/catch around `retry.onResponse`, so the raw `TypeError` (or other fetch error) surfaces to the caller instead of a normalized `NetworkError`. The `onError` normalization introduced by this PR only covers the initial fetch, not retries initiated from `onResponse`. A straightforward guard would be to wrap the `retry.onResponse` call in `executeManual` with a try/catch that calls `toNetworkError` on any thrown error.

### Issue 2 of 4
sdks/typescript-sdk/src/client/retry-policy.ts:92-98
`parseRetryAfterMs` has no upper bound. A server returning `Retry-After: 86400` would cause the SDK to sleep for 24 hours inside `RetryInterceptor.onResponse`'s backoff loop. The `timeoutMs` guard only wraps individual `fetch` calls — not the `sleep` — so a misbehaving or adversarial upstream can hold the caller process indefinitely. Adding a reasonable cap (e.g., 60 seconds) prevents this operational hazard.

```suggestion
  const MAX_RETRY_AFTER_MS = 60_000; // 60 s cap — prevent runaway sleeps

  if (/^\d+$/.test(trimmed)) {
    return Math.min(Number(trimmed) * 1000, MAX_RETRY_AFTER_MS);
  }

  const dateMs = Date.parse(trimmed);
  if (Number.isNaN(dateMs)) return undefined;
  return Math.min(Math.max(0, dateMs - now), MAX_RETRY_AFTER_MS);
```

### Issue 3 of 4
sdks/typescript-sdk/src/client/retry-policy.ts:51-53
`if (error instanceof TypeError) return true` is too broad: it matches any `TypeError` — including programming bugs inside the fetch implementation (e.g., `Cannot read property 'x' of undefined`) — rather than just network-level failures. This would mask real bugs by retrying them up to `maxRetries` times. A tighter guard that also inspects the error message keeps the intent while reducing false positives.

```suggestion
  // undici/whatwg surface generic connection failures as a TypeError whose
  // message is "fetch failed" (often with a `cause`).
  if (error instanceof TypeError && /fetch failed|network/i.test(err.message ?? '')) return true;
```

### Issue 4 of 4
sdks/typescript-sdk/src/client/retry-policy.ts:7-8
**`PUT` and `DELETE` excluded from idempotent methods**

`IDEMPOTENT_METHODS` only contains `GET`, `HEAD`, and `OPTIONS`. Both `PUT` and `DELETE` are idempotent per RFC 7231 — repeated calls produce the same server state. SDK operations such as `stopTrackingShipment` (likely `DELETE`) and `updateShipment`/`updateTrackingRequest` (likely `PATCH` or `PUT`) will surface an `UpstreamError` immediately on a transient 5xx with no retry, even though replaying them is safe. Treating `PUT` and `DELETE` as non-idempotent is conservative but may surprise callers who expect automatic resilience on those paths.

Reviews (1): Last reviewed commit: "docs(sdk): regenerate reference for tran..." | Re-trigger Greptile

Greptile also left 4 inline comments on this PR.

@linear-code

linear-code Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

DEV-10663

@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
api Ready Ready Preview, Comment Jul 1, 2026 8:42pm

Request Review

@mintlify

mintlify Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
terminal49 🟢 Ready View Preview Jun 24, 2026, 6:18 PM

@dodeja
dodeja marked this pull request as ready for review June 26, 2026 00:24

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d6f00ef4fc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdks/typescript-sdk/src/client/managers/base.ts
Comment thread sdks/typescript-sdk/src/client/transport.ts Outdated
Comment thread sdks/typescript-sdk/src/client/retry-policy.ts Outdated
Comment thread sdks/typescript-sdk/src/client/retry-policy.ts Outdated
Comment thread sdks/typescript-sdk/src/client/retry-policy.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 257d4da83b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdks/typescript-sdk/src/client/timeout.ts
Comment thread sdks/typescript-sdk/src/client/interceptors.ts Outdated
Comment thread sdks/typescript-sdk/src/client/interceptors.ts Outdated
@dodeja

dodeja commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Triaged the automated review feedback (Codex + Greptile) plus the known residuals. Most items were already handled in 257d4da; this push (51a9ad5) closes the two remaining real gaps in the typed-client retry path.

Addressed now (51a9ad5):

  • Uncaught network error inside onResponse retry (Greptile P1 / Codex P2 on interceptors.ts/transport.ts): a network failure thrown by a response-triggered retry surfaced as a raw TypeError because openapi-fetch does not route an onResponse throw back through onError. Now wrapped and normalized via toNetworkError, matching the initial-fetch path. (The manual /search path was already fixed in 257d4da; this covers the typed-client path.)
  • Replay state dropped across onError -> onResponse (Codex P2, interceptors.ts ~L146): onError deleted the replay entry in its finally even when it returned a recovered Response, so the subsequent onResponse could not retry a following 429/5xx. A transient network-fail -> 500 -> success sequence now succeeds with remaining budget. Replay state is only cleared on terminal error paths; the onResponse chain clears it once settled.
  • Added transport unit tests for both (verified red against the prior code). Also dropped a now-unneeded type cast in the iterate() test, confirming maxPages/maxRows are type-reachable.

Already addressed in 257d4da (no further change):

  • maxPages/maxRows exposed on ListOptions and threaded into iterate() (residual a) + docs regenerated.
  • /search executeManual comment explaining the absent OpenAPI route (residual b).
  • Retry-After capped at 60s (MAX_RETRY_AFTER_MS).
  • Tightened TypeError network-error detection (message must look network-ish).

Intentionally skipped:

  • Add PUT/DELETE to idempotent methods (Greptile P2): out of scope / design decision. The SDK's actual write ops (update, stopTracking, resumeTracking) are all PATCH, not PUT/DELETE, so this would not help them; non-idempotent writes already have an opt-in Idempotency-Key retry path. Broadening auto-retry to DELETE/PUT is a deliberate safety choice best left to a product decision.
  • Keep timeout active while the response body is read (Codex P2, timeout.ts): real limitation but higher-risk. The AbortController bounds the fetch; extending the timer through body reads requires wrapping the response body stream, which would interact with the clone/read semantics the error-mapping and retry interceptors depend on. Out of scope for this PR.

Green gate from the worktree: SDK 85 pass / 2 skip, MCP 77 pass; both builds + type-checks clean. No public SDK surface change in this commit, so no docs regeneration needed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 51a9ad5638

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdks/typescript-sdk/src/client/interceptors.ts
dodeja and others added 4 commits July 1, 2026 13:38
…ly retries, Retry-After, bounded iterate

Harden the TypeScript SDK transport against the failure modes that can
wedge or corrupt a caller in production:

- Request timeout: every request is now bounded by an AbortController-based
  timeout (configurable via `timeoutMs`, default 30s, `0` disables). A hung
  upstream is aborted and rejected with a new `TimeoutError`. Applied once in
  Transport so it covers both the openapi-fetch client and `executeManual`.
  (new `client/timeout.ts`)

- Network-error normalization + retry: a thrown `fetch` failure (DNS,
  ECONNRESET, "fetch failed", ...) is normalized to a `Terminal49Error`
  (`NetworkError`) and run through the same retry policy as a 5xx, via the
  RetryInterceptor `onError` hook. (`errors.ts`, `interceptors.ts`)

- Idempotent-only retries: retries are gated to GET/HEAD (and writes that
  carry an `Idempotency-Key`). A POST/PATCH that hits a transient 5xx/network
  error is no longer silently replayed, which could have created duplicate
  tracking requests. (new pure `client/retry-policy.ts`)

- Retry-After: 429 backoff honors the server `Retry-After` header (delta-seconds
  or HTTP-date) instead of fixed exponential only.

- Bounded iterate(): `BaseManager.createIterator` now stops at documented
  `maxPages` / `maxRows` safety caps so a no-op/overly-broad filter cannot walk
  the entire dataset. (`managers/base.ts`)

- Search path: `executeManual` reads success bodies with `readSuccessBody`
  (new `client/body.ts`) so a non-JSON 200 is surfaced instead of being
  silently collapsed to `undefined`, and a thrown network error is normalized.

- Documented the load-bearing interceptor registration order (Retry registered
  last so it runs before error-mapping on the reverse onResponse/onError pass).

Pure logic is extracted into unit-testable modules (retry-policy, timeout,
body). New + updated mock-transport tests cover: timeout abort, network-error
normalize+retry, Retry-After wait, write-not-retried, and the iterate bound.

Closes DEV-10663

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…manual-path error normalization

Addresses review feedback on the transport-resilience PR:

- Expose `maxPages`/`maxRows` on the public `iterate()` signatures by adding
  them to `ListOptions`, so TypeScript callers can raise the iterator safety
  caps with types (previously type-unreachable). Regenerated SDK reference docs
  for the new `ListOptions` properties.
- Normalize a network error thrown from a retry kicked off inside
  `RetryInterceptor.onResponse` in the manual `/search` path: wrap the
  `retry.onResponse` call in `executeManual` so the caller always sees a
  `NetworkError` rather than a raw `TypeError`.
- Cap a honored `Retry-After` delay at 60s (`MAX_RETRY_AFTER_MS`) so an
  adversarial/misbehaving upstream cannot wedge the caller in a multi-hour sleep
  (the request timeout guards `fetch`, not the backoff sleep).
- Tighten network-error detection: a bare `TypeError` is no longer treated as
  retryable unless its message looks network-ish, so a programming-bug
  `TypeError` is not retried up to `maxRetries` and masked.
- Document why `search()` uses `executeManual` (no `/search` entry in the
  generated OpenAPI types, so it cannot route through the typed client).

Added unit tests for the Retry-After cap and the tightened TypeError guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… state across onError->onResponse

Two transport-resilience gaps in the typed-client retry path remained after the
prior review pass:

- A network error thrown by a response-triggered retry inside
  `RetryInterceptor.onResponse` propagated as a raw `TypeError`. openapi-fetch
  does not route an `onResponse` throw back through `onError`, so the caller saw
  an un-normalized error instead of a `NetworkError`. Wrap the retry `fetch` and
  normalize via `toNetworkError`, matching the initial-fetch path.
- `onError` deleted the replay entry in its `finally` even when it returned a
  recovered Response. openapi-fetch then runs `onResponse` for that same request
  id, but with the replay state gone it could not retry a subsequent 429/5xx, so
  a transient network-failure -> 500 -> success sequence failed despite remaining
  retry budget. Only delete the replay entry on the terminal error paths; the
  `onResponse` chain deletes it once the response is settled.

Added transport tests for both paths (verified red against the prior code).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dodeja
dodeja force-pushed the fix/sdk-transport-resilience branch from 51a9ad5 to dcfd05d Compare July 1, 2026 20:41
@dodeja

dodeja commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (was 10 commits behind: MCP spec-adherence, transport-events docs, container/shipment filter fixes, etc.) — rebase applied cleanly with no conflicts.

Re-audited all 9 inline Codex/Greptile review comments against the current code on the branch tip (51a9ad5, now dcfd05d after rebase) rather than trusting the prior "Addressed" replies at face value:

Verified fixed in code:

  • Response-triggered retry network errors now normalized via toNetworkError in both RetryInterceptor.onResponse (interceptors.ts) and Transport.executeManual (transport.ts).
  • Replay state is preserved across onError -> onResponse recovery (only cleared on terminal error paths), so a network-fail -> 500 -> success sequence retries correctly.
  • parseRetryAfterMs clamps to MAX_RETRY_AFTER_MS = 60_000 on both the delta-seconds and HTTP-date branches.
  • isRetryableNetworkError no longer treats every TypeError as retryable — requires a network-ish message (fetch failed|network|socket hang up|terminated) in addition to the error-code allow-list.
  • iterate() on shipments/containers/tracking-requests takes Omit<ListOptions, 'page'>, and ListOptions carries maxPages/maxRows through to BaseManager.createIterator — callers can raise the caps without a cast (regression test in client.transport.test.ts).

Confirmed intentional skips (documented in thread replies, still correct):

  • PUT/DELETE excluded from auto-retried idempotent methods — conservative default; non-idempotent writes get an explicit Idempotency-Key opt-in instead.
  • Timeout doesn't cover slow body streaming after headers arrive — real but separate concern (body-read deadline vs. request timeout), out of scope for this pass.
  • maxRetries is effectively per-phase (network-error phase, then HTTP-status phase) rather than one global counter — deliberate trade-off to make network-fail -> 500 -> success recoverable; each phase is still capped and gated by idempotency + backoff.

No further code changes were needed — everything substantive had already landed in 257d4da / 51a9ad5. This push is the rebase only.

Checks after rebase (from the worktree):

  • npm run test --workspace @terminal49/sdk -- --run — 95 passed, 2 skipped
  • npm run build --workspace @terminal49/sdk — clean
  • npm run lint --workspace @terminal49/sdk (oxlint + oxfmt --check) — clean

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dcfd05df63

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// would otherwise surface as a raw TypeError. Normalize it to a
// NetworkError so the caller sees the same error shape as the
// initial-fetch path.
throw toNetworkError(error);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep retrying retryable failures during response replay

When a 429/5xx response triggers this retry loop, any network failure from the replayed fetch is thrown immediately, so a sequence like 503 -> TypeError('fetch failed') -> 200 fails even with maxRetries: 2. Fresh evidence in the current diff is that the catch still exits via throw toNetworkError(error) before incrementing the attempt or checking whether remaining retry budget can cover a retryable network failure.

Useful? React with 👍 / 👎.

Comment on lines +147 to +149
} catch (retryError) {
attempt++;
if (attempt >= this.maxRetries) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Stop replaying after non-retryable retry errors

After the initial error passes isRetryableNetworkError, subsequent retry failures are retried unconditionally until the budget is exhausted. If the first replay is aborted by the SDK timeout or caller cancellation, or throws a non-network TypeError, maxRetries > 1 will still sleep and replay again even though the retry policy explicitly treats those errors as non-retryable; re-check retryError before continuing the loop.

Useful? React with 👍 / 👎.

@dodeja
dodeja merged commit e9ac7a8 into main Jul 1, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant